Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement MobX #63

Closed
wants to merge 3 commits into from
Closed

Implement MobX #63

wants to merge 3 commits into from

Conversation

samcooke98
Copy link
Contributor

I noticed the mobX tests went walkabout. I added them back, mainly cause I was interested to see how MobX goes.

@@ -18,7 +18,11 @@ document.title = name;

// concurrent mode
const root = ReactDOM.createRoot(document.getElementById('app'));
root.render(<App />);
root.render(
<React.StrictMode>
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The React Profiler was complaining about StrictMode being missing, so I added it here. Not sure if you would prefer this in a separate PR.

@dai-shi
Copy link
Owner

dai-shi commented Feb 26, 2022

Hi, the reason why mobx is removed is because the impl didn't use the common reducer. Can you try something like valtio?

@samcooke98
Copy link
Contributor Author

Hi, the reason why mobx is removed is because the impl didn't use the common reducer. Can you try something like valtio?

Ah interesting.

Can you try something like valtio?

The reason I wanted to introduce MobX is that it's the chose state management library at the company I work for.

I could maybe make it try and use the reducer?

@dai-shi
Copy link
Owner

dai-shi commented Feb 27, 2022

Yeah, it would be more maintainable if we were to merge this.
Otherwise, feel free to run all tests on your end and update the results. (Leaving this PR open for reference.)

@kubk
Copy link

kubk commented Apr 13, 2022

Is it going to be merged? Why the impl have to use a common reducer?

@dai-shi
Copy link
Owner

dai-shi commented Apr 13, 2022

Because we might be changing the common reducer in the future. Would you like to give it a try?

@kubk
Copy link

kubk commented Apr 14, 2022

@dai-shi Thanks, it's clear 👍

@samcooke98 Hi, are you going change the implementation to use the shared reducer?

@dai-shi
Copy link
Owner

dai-shi commented May 17, 2022

@kubk Feel free to work on it (please open a new PR).

Closing as stale.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants